-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] AI/LLM integration #1325
base: develop
Are you sure you want to change the base?
[WIP] AI/LLM integration #1325
Conversation
@@ -369,6 +369,11 @@ class NoteContext extends Component implements EventListener<"entitiesReloaded"> | |||
|
|||
const { note, viewScope } = this; | |||
|
|||
// For llmChat viewMode, show a custom title | |||
if (viewScope?.viewMode === "llmChat") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a specific view mode for chats? The idea is that view modes are supposed to be pretty generic like view sources, in app help, attachments.
A good view mode candidate would be to view the text representation of notes, to see what the LLM "sees".
|
||
try { | ||
// We'll use the Note Map approach - open a known note ID that corresponds to the LLM chat panel | ||
await appContext.tabManager.openTabWithNoteWithHoisting("_globalNoteMap", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to go with the note map approach, then we need a note specific for the LLM chat in the hidden subtree with a normal view.
src/public/app/desktop.ts
Outdated
@@ -27,6 +28,16 @@ bundleService.getWidgetBundlesByParent().then(async (widgetBundles) => { | |||
}); | |||
console.error("Critical error occured", e); | |||
}); | |||
|
|||
// Initialize right pane tab manager after layout is loaded | |||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels kind of like a hack or a work-around, doesn't it? What problem does it solve?
// Initialize the right pane tab manager after widget render | ||
setTimeout(() => { | ||
const $tabContainer = $("#right-pane-tab-container"); | ||
const $contentContainer = $("#right-pane-content-container"); | ||
|
||
if ($tabContainer.length && $contentContainer.length) { | ||
rightPaneTabManager.init($tabContainer, $contentContainer); | ||
} | ||
}, 500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, why are we running the code behind a delay?
@@ -0,0 +1,13 @@ | |||
function initComponents() { | |||
// ... existing code ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the existing code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏃💨
if (query.toLowerCase().includes("provide details about") || | ||
query.toLowerCase().includes("information related to")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should extract all these strings to translatable strings, or at least gather them as constants in a single place.
We need to be able to support multiple languages in the future, especially now that we added internationalization support to Trilium.
// Simple heuristics for common languages | ||
if (firstLines.includes('<?php')) return 'php'; | ||
if (firstLines.includes('#!/usr/bin/python') || firstLines.includes('import ') && firstLines.includes('def ')) return 'python'; | ||
if (firstLines.includes('#!/bin/bash') || firstLines.includes('#!/usr/bin/bash')) return 'bash'; | ||
if (firstLines.includes('#!/usr/bin/perl')) return 'perl'; | ||
if (firstLines.includes('#!/usr/bin/ruby')) return 'ruby'; | ||
if (firstLines.includes('package ') && firstLines.includes('import ') && firstLines.includes('public class ')) return 'java'; | ||
if (firstLines.includes('using System;') && firstLines.includes('namespace ')) return 'csharp'; | ||
if (firstLines.includes('package main') && firstLines.includes('import (') && firstLines.includes('func ')) return 'go'; | ||
if (firstLines.includes('#include <') && (firstLines.includes('int main(') || firstLines.includes('void main('))) { | ||
if (firstLines.includes('std::')) return 'cpp'; | ||
return 'c'; | ||
} | ||
if (firstLines.includes('fn main()') && firstLines.includes('let ') && firstLines.includes('impl ')) return 'rust'; | ||
if (firstLines.includes('<!DOCTYPE html>') || firstLines.includes('<html>')) return 'html'; | ||
if (firstLines.includes('function ') && firstLines.includes('var ') && firstLines.includes('const ')) return 'javascript'; | ||
if (firstLines.includes('interface ') && firstLines.includes('export class ')) return 'typescript'; | ||
if (firstLines.includes('@Component') || firstLines.includes('import { Component }')) return 'typescript'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add a TODO to integrate Highlight.js to do this sort of thing on its own, without having to hard-code it on our side.
const CONTEXT_WINDOW = { | ||
OPENAI: 16000, | ||
ANTHROPIC: 100000, | ||
OLLAMA: 8000, | ||
DEFAULT: 4000 | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, here it would be nice to define how these values were obtained and how we can maintain them, e.g. if we add a new AI provider.
import cls from "../../../../services/cls.js"; | ||
import type { NoteEmbeddingContext } from "../types.js"; | ||
// Remove static imports that cause circular dependencies | ||
// import { storeNoteEmbedding, deleteNoteEmbeddings } from "./storage.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code must be removed.
@@ -9,7 +9,7 @@ import searchService from "./search/services/search.js"; | |||
import SearchContext from "./search/search_context.js"; | |||
import hiddenSubtree from "./hidden_subtree.js"; | |||
import { t } from "i18next"; | |||
const { LBTPL_NOTE_LAUNCHER, LBTPL_CUSTOM_WIDGET, LBTPL_SPACER, LBTPL_SCRIPT } = hiddenSubtree; | |||
const { LBTPL_NOTE, LBTPL_CUSTOM_WIDGET, LBTPL_SPACER, LBTPL_SCRIPT } = hiddenSubtree; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
just a general comment: I saw quite a few uses of "any" as type It looks like for some of these that should be rather trivial :-)
The idea is to at some point have 0 uses of any :-) |
const ollamaBaseUrl = baseUrl || await options.getOption('ollamaBaseUrl') || 'http://localhost:11434'; | ||
|
||
// Call Ollama API to get models | ||
const response = await axios.get(`${ollamaBaseUrl}/api/tags?format=json`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose to get rid of axios
and just stick to the built-in fetch
– from what I saw, we only have that depdency for the backend_script_api, where "axios" is marked as deprecated.
/**
* Axios library for HTTP requests. See {@link https://axios-http.com} for documentation
* @deprecated use native (browser compatible) fetch() instead
*/
axios: typeof axios;
@@ -369,6 +374,48 @@ function register(app: express.Application) { | |||
etapiSpecRoute.register(router); | |||
etapiBackupRoute.register(router); | |||
|
|||
// Embeddings API endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea about LLMs and all, but are "embeddings" something LLM specific?
if yes, wouldn't it make sense to have the API path reflect this as well?
e.g. by changing /api/embeddings/
→ /api/llm/embeddings/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An embedding is something like a baked croissant. Different bakers can make them with varying quality, but if you give the croissant to your friend at work, they may (or may not) think this croissant is better than one by another bakery.
Long-winded way of saying that even though LLMs generate them, but we use them locally to do cosine similarity computation to see what notes to include at query time and don’t directly provide the embedding to the LLM we’re talking to.
I’m not sure if you’re asking to move the endpoint to /api/<llm>/embeddings
or /api/llm/embeddings
- but yeah I think the latter makes more sense
apiRoute(GET, "/api/embeddings/index-rebuild-status", embeddingsRoute.getIndexRebuildStatus); | ||
|
||
// LLM chat session management endpoints | ||
apiRoute(PST, "/api/llm/sessions", llmRoute.createSession); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't these missing the CSRF/Auth middleware – or am I missing something
apiRoute(PST, "/api/llm/index/notes/:noteId", llmRoute.indexNote); | ||
|
||
// Ollama API endpoints | ||
route(PST, "/api/ollama/list-models", [auth.checkApiAuth, csrfMiddleware], ollamaRoute.listModels, apiResultHandler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here:
Generally speaking:
I think it would make it cleaner to have these under /api/llm/
if they are LLM specific.
also apart from the above – I wonder here:
wouldn't it make sense to have the route be list-models/${LLM}
instead?
at least from a REST point of view, I feel this would "group" these more logically.
|
||
const { note } = notes.createNewNote({ | ||
parentNoteId: rootNoteId, | ||
title: title || 'New Chat ' + now.toLocaleString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to have the "default fallback string" translatable?
parentNoteId: 'root', | ||
title: 'AI Chats', | ||
type: 'text', | ||
content: 'This note contains your saved AI chat conversations.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these be translatable?
@@ -0,0 +1,433 @@ | |||
/** | |||
* Helper functions for processing code notes, including language detection and structure extraction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a general question: what is the purpose of this code_handlers exactly – I can see some attempt to extract structure from code notes – aren't all those LLM models "smart enough" to do that themselves?
Again – not a LLM expert, so excuse me if that is a stupid question :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not - all that LLMs “see” is text, so we have to build as much of a text representation of the Note (regardless of type) as we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps for the code note type specifically, yes, we can provide the vast majority of the Note content directly to the LLM we’re talking- we’d just have to clean up any tags to minimize its impact on the size of the context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’re performing those actions on code notes to try to extract what’s useful to minimize impact on the size of the context.
|
||
// Add note about truncation if needed | ||
if (childNotes.length > maxChildren) { | ||
context += `... and ${childNotes.length - maxChildren} more child notes not shown\n`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be translatable, please :-)
Status:

Goals:
Out of scope: